Skip to content

[PM-26292] tool: Add script to delete unused Localizable.strings entries#2464

Open
KatherineInCode wants to merge 6 commits intomainfrom
pm-26292/delete-unused-strings
Open

[PM-26292] tool: Add script to delete unused Localizable.strings entries#2464
KatherineInCode wants to merge 6 commits intomainfrom
pm-26292/delete-unused-strings

Conversation

@KatherineInCode
Copy link
Copy Markdown
Contributor

@KatherineInCode KatherineInCode commented Mar 18, 2026

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-26292

📔 Objective

This builds on #2412 by adding a module that finds unused strings and deletes them. I also did some refactoring of the strings file management into a separate python module.

I also ran the script, and it found 308 unused keys, many of which were identified in #2003. I'm not entirely sure why it's so many, especially of what seem like basic strings, though my guess is that some are used on Android, and a lot of them are detritus from MAUI or earlier that just never got cleaned up.

As noted in #2412, eventually we could have running these scripts be part of the CI process in some way, but currently these are still manual runs.

@github-actions github-actions bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context t:ci Change Type - Updates to automated workflows labels Mar 18, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 18, 2026 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – 2a04be39-04cb-41a2-b02c-ed6a41897f88

Great job! No new security vulnerabilities introduced in this pull request

@KatherineInCode KatherineInCode added the ai-review Request a Claude code review label Mar 18, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 18, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.95%. Comparing base (c342e82) to head (88c53c4).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2464      +/-   ##
==========================================
- Coverage   87.08%   85.95%   -1.13%     
==========================================
  Files        1862     2092     +230     
  Lines      165404   180212   +14808     
==========================================
+ Hits       144042   154904   +10862     
- Misses      21362    25308    +3946

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@KatherineInCode KatherineInCode mentioned this pull request Mar 18, 2026
Merged
@KatherineInCode KatherineInCode changed the title [PM-26292] Add script to delete unused Localizable.strings entries [PM-26292] tool: Add script to delete unused Localizable.strings entries Mar 19, 2026
@KatherineInCode KatherineInCode added the innovation Feature work related to Innovation Sprint or BEEEP projects label Mar 31, 2026
Base automatically changed from pm-26293/delete-unused-strings to main April 9, 2026 14:15
@KatherineInCode KatherineInCode force-pushed the pm-26292/delete-unused-strings branch from f25e6b7 to 1b5766f Compare April 9, 2026 14:32
@KatherineInCode KatherineInCode marked this pull request as ready for review April 9, 2026 14:32
@KatherineInCode KatherineInCode requested review from a team and matt-livefront as code owners April 9, 2026 14:32
@KatherineInCode KatherineInCode force-pushed the pm-26292/delete-unused-strings branch from 1b5766f to 688112f Compare April 9, 2026 15:04
@KatherineInCode KatherineInCode force-pushed the pm-26292/delete-unused-strings branch from 688112f to 87d355a Compare April 9, 2026 15:10
@github-actions github-actions bot added t:feature and removed app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context labels Apr 9, 2026
@github-actions github-actions bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context labels Apr 9, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026 •
edited
Loading

Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the new delete-unused Python script, the strings_file_utils refactoring, shell script integration, and the removal of 308 unused localization keys from Localizable.strings. The detection strategy correctly scans for Localizations.X references (the SwiftGen-generated access pattern) and normalizes keys to account for SwiftGen's identifier stripping. Confirmed no alternative access patterns (e.g., direct NSLocalizedString calls) exist in the codebase that could cause false deletions. Test coverage is comprehensive across unit and integration levels.

vvolkgang
vvolkgang reviewed Apr 9, 2026
View reviewed changes
Scripts/fix-localizable-strings/main.py
if args.dry_run:
with open(args.strings, encoding="utf-8") as f:
content = f.read()
import os
Copy link
Copy Markdown
Member

@vvolkgang vvolkgang Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 suggestion - moving inline imports to the top of the scripts, makes it easier to know in advance what the script is using. There's another in the test_delete_unused_strings.py‎ file (import shutil).

vvolkgang
vvolkgang reviewed Apr 9, 2026
View reviewed changes
Scripts/fix-localizable-strings/delete_unused_strings.py


def _normalize_key(key: str) -> str:
"""Normalize a ``.strings`` key for comparison against a SwiftGen identifier.
Copy link
Copy Markdown
Member

@vvolkgang vvolkgang Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Very personal take - given these scripts aren't for public distribution, these docstrings are blowing up line count without adding much, method name and the 3 comments above the regexps were enough for me at least.

vvolkgang
vvolkgang requested changes Apr 9, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@vvolkgang vvolkgang left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments but nothing major here, we're just waiting for the Localizable.strings file to be updated following the merge of #2003

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vvolkgang vvolkgang vvolkgang requested changes

@matt-livefront matt-livefront Awaiting requested review from matt-livefront matt-livefront is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

ai-review Request a Claude code review app:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context innovation Feature work related to Innovation Sprint or BEEEP projects t:ci Change Type - Updates to automated workflows t:feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@KatherineInCode @vvolkgang